Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scissors - Laurel O #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

lolkinetzky
Copy link

No description provided.

Comment on lines +24 to +25
list_com = [v for i in user_data_user_data_key for k,v in i.items() \
if v == i[value]]
Copy link

@jbieniosek jbieniosek Mar 31, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Variable names can greatly improve code readability.

  2. When using list comprehension, simpler is often more readable: list_com = [i[value] for i in user_data_user_data_key]. For readability I recommend avoiding double loops in list comprehension (sometimes they are unavoidable, but they are much harder to read).

compare this function to (please ignore tab error):

def generate_list_from_key(movie_list, dict_key):
    new_list = [movie[dict_key] for movie in movie_list]
    return new_list

list_dic = user_data["watched"]
avg_rating = 0.0
if len(list_dic) > 0:
rating_list = [v for i in list_dic for k,v in i.items() if k=="rating"]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rating_list = [movie["rating"] for movie in list_dic]

genres = list_com_fun(user_data["watched"], "genre")
if genres == []:
return None
return max(genres)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you solve this if you didn't have access to the max function?

Comment on lines +47 to +48
friend1 = list_com_fun(user_data["friends"][0]["watched"] , "title")
friend2 = list_com_fun(user_data["friends"][1]["watched"] ,"title")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function will pass the tests because they only have 2 friends, how would this function need to change to be generalized to a friend list of any size?

movie = {"title":v}
movie_copy = movie.copy()
answer.append(movie_copy)
return answer

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests for this function use a simplified version of the movie dictionary. However, our original definition of a movie was that a movie has a title, rating and genre. How could you create a list of the original movie dictionaries using the unique list?

Note: movie.copy() is not needed here.


def get_available_recs(user_data):
user_sub = user_data["subscriptions"]
friend_sub = [i for i in user_data["friends"][0]["watched"] and \

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has the same limitation as get_unique_watched and get_friends_unique_watched. How could you account for an unknown number of friends?

return rec_host

def get_new_rec_by_genre(user_data):
fav_genre = get_most_watched_genre(user_data)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great use of a helper function!

@jbieniosek
Copy link

Great work on this project! Some things to work on in the next project:

  1. Consider what the problem is asking, not just the test. The test is there to check if the function solves the problem, but the function should not solve only the test. Consider the ecosystem in which the function lives. Are there cases that the tests don't cover?
  2. Focus on readability. List comprehensions are great, unless they take away from the readability of the code. Additionally, variable and function names can make or break the readability of code. The shortest version of the code is not always the most readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants